chore(evaluation): type strict - #1121
Conversation
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
changed the title
AkhileshNegi
changed the title
OpenAPI changes ⚪ No API surface changesNote This PR does not modify the API contract.
|
AkhileshNegi
changed the title
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
| def upload_jsonl_to_object_store( | ||
| storage: CloudStorage, | ||
| results: list[dict], | ||
| results: Sequence[Mapping[str, Any]], |
There was a problem hiding this comment.
Any type? if possible, can we try to avoid the any type here or every places? need to do proper type declaration.
| import logging | ||
| from typing import Any | ||
| from collections.abc import Mapping, Sequence | ||
| from typing import Any, cast |
| if saved is not None: | ||
| eval_run = saved | ||
| eval_run.score = score | ||
| eval_run.score = cast(dict[str, Any], score) |
There was a problem hiding this comment.
try to avoid the any type declaration.
| # rollup stays in sync with the new traces, not just the cached ones. | ||
| # `_attach_category_metrics` mutates in place and is idempotent. | ||
| _attach_category_metrics(merged_score) | ||
| _attach_category_metrics(cast(dict[str, Any], merged_score)) |
There was a problem hiding this comment.
why added the any type declaration?
|
|
||
| if eval_run: | ||
| eval_run.score = merged_score | ||
| eval_run.score = cast(dict[str, Any], merged_score) |
| db_score = {"summary_scores": summary_score} | ||
| if score.get("overall") is not None: | ||
| db_score["overall"] = score["overall"] | ||
| db_score: dict[str, Any] = {"summary_scores": summary_score} |
There was a problem hiding this comment.
try to avoid use the Any type declaration.
| logger.warning( | ||
| "[update_traces_with_cosine_scores] " | ||
| f"Score item missing cosine_similarity, skipping | trace_id={trace_id}" | ||
| ) |
There was a problem hiding this comment.
do we actually need this logger? i am not sure it provides much value since most of this information can already be tracked from the database, and it’s unlikely that we will regularly need to check this through logs.
There was a problem hiding this comment.
i would suggest removing it for now and adding it back later if we identify a specific debugging or monitoring need for it. what do you think?
|
also, codecov report checks are failing. so please fix those too. |
2 participants
Issue
Part of #1083
Summary
TypedDictpayloads passed asdict[str, Any],Optionalvalues used without narrowing.Nonepaths the checker exposed are handled.Types:
col()inorder_by/is_not; typed score payloads (EvaluationScore,SummaryScore,TraceData) threaded through merge/summary/processing/fast; read-only args widened toSequence/Mapping;TraceData.category→NotRequired;failure_response→APIResponse[T].Behaviour (only runtime-affecting bits): skip traces with
Nonequestion_id/ missingitem_id; skip + warn on missingcosine_similarityinstead of sendingNoneto Langfuse; readstring_valuefor Langfuse text scores; fail the run explicitly when config/provider is unresolvable; raisetrace_download_failedwhenscore_trace_urlis missing.Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Notes
Evaluation modules only — TTS/STT from #1083 not covered here.